Repository navigation
fix(powershell): keep the activation and env output ASCII - #762
Open
linghengqian wants to merge 5 commits into
Open
linghengqian wants to merge 5 commits into
linghengqian wants to merge 5 commits into
Conversation
Windows PowerShell and PowerShell 7 decode the output of a native command with the console's code page before the hook can switch the console to UTF-8, so the raw UTF-8 that vfox activate prints comes out garbled on a Chinese system (version-fox#689). The unit tests assert the output is ASCII and that the escaped quotes mean the value; the Windows test runs both PowerShells on code page 936 with a non-ASCII PATH entry and a non-ASCII vfox path.
…ns job The build matrix cancels its other legs when one fails, so a run of the Windows build leg cannot be relied on to report on its own. The regressions job runs its Windows tests independently of the matrix.
PowerShell decodes the output of a native command with the console's code page, and the hook sets the console to UTF-8 only after that output has been decoded. vfox printed the non-ASCII text of PATH, the other variables and the vfox path as raw UTF-8, which came out garbled on a Chinese system, so the session started with a PATH that had lost characters and sometimes a hook that did not parse (version-fox#689). PowerShellEscape now writes printable ASCII as a single-quoted literal, with the quote doubled as PowerShell requires, and everything else as the base64 of its UTF-8 bytes, decoded when the expression runs. Names that $env: cannot spell go through [System.Environment]. The vfox path in the hook is written the same way, and a path that contains {{ takes the encoded form, since text/template would read it as an action. This also fixes values with an apostrophe or a control character, which the old escaper wrote as text PowerShell reads differently or cannot parse.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #762 +/- ##
==========================================
+ Coverage 56.62% 58.34% +1.71%
==========================================
Files 116 123 +7
Lines 8595 8884 +289
==========================================
+ Hits 4867 5183 +316
+ Misses 3263 3228 -35
- Partials 465 473 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
linghengqian
commented
Nov 14, 2026
linghengqian
left a comment
linghengqian
left a comment
Member
Author
There was a problem hiding this comment.
CC @version-fox/vfox-maintainers @version-fox/vfox-committers . Could you please take a look and provide a review?
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Approval should wait for Windows CI or human validation because PowerShell 5.1/7 native-command decoding, code-page 936 behavior, and executable invocation semantics cannot be runtime-verified in this review container.
No open issues 🎉
What changed in this PR
This PR fixes PowerShell activation and environment export corruption on non-UTF-8 Windows code pages by generating ASCII-only scripts while preserving Unicode values through UTF-8/base64 decoding.
Changes:
- Reworked PowerShell escaping, environment assignment, and executable-path handling.
- Added unit, activation, and Windows integration coverage for Unicode paths, quotes, control characters, and code page 936.
- Updated project guidance and Windows CI with the ASCII-output requirement and regression test.
| File | Description |
|---|---|
| internal/shell/powershell.go | Implements ASCII-safe PowerShell escaping, environment assignment, and activation-path handling. |
| internal/shell/powershell_test.go | Tests PowerShell escaping, value round-tripping, exports, template safety, and ASCII output. |
| cmd/commands/powershell_cli_test.go | Adds Windows PowerShell integration coverage for non-ASCII paths under code page 936. |
| cmd/commands/nushell_cli_test.go | Uses the .exe extension when building the Windows test binary. |
| cmd/commands/activate_test.go | Verifies rendered PowerShell activation scripts remain ASCII. |
| AGENTS.md | Documents the PowerShell ASCII-output requirement. |
| .github/workflows/ci.yml | Runs the PowerShell regression test in the Windows CI workflow. |
Review effort: Lite
aooohan
reviewed
Nov 15, 2026
aooohan
left a comment
aooohan
left a comment
Member
There was a problem hiding this comment.
Reviewed 17e7acc. I found one low-priority issue in the unset handling, noted inline. I did not find a blocking issue in the ASCII-only activation approach; I recommend fixing the small unset issue and adding a native regression test before merging.
Validation:
go test ./internal/shell ./cmd/commandspassed on macOS with Go 1.24.0.- Additional native PowerShell 7.6.6 checks passed for activation and prompt-driven version changes with Unicode, apostrophes,
{{...}}, dollar signs, and backticks in executable paths. - I inspected the Windows regression CI logs, which confirm that both Windows PowerShell 5.1 and PowerShell 7 executed and passed the non-ASCII PATH test. Windows execution was verified through those logs, not rerun locally.
The text assertions only see the text Export writes. The new test runs the export and then the unset of a name in each PowerShell on the machine. For WITH.DOT, PowerShell 7.5 and 7.7 keep the variable with an empty value after SetEnvironmentVariable(name, $null), so the test fails there; PowerShell 7.4 removes it and passes.
From PowerShell 7.5, SetEnvironmentVariable(name, $null) empties the variable instead of removing it, so unsetting WITH.DOT left the variable in the session with an empty value. The unset now passes [NullString]::Value, which removes it on PowerShell 7.4, 7.5 and 7.7. TestPwshUnsetRemovesVariableInPowerShell checks the session itself, which the text assertion could not.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #689
What changed
internal/shell/powershell.go:PowerShellEscapewrites printable ASCII as a single-quoted literal, with'doubled. Any other text is written as([System.Text.Encoding]::UTF8.GetString([System.Convert]::FromBase64String('...'))), which decodes the base64 of its UTF-8 bytes when it runs. The output ofvfox activate pwshandvfox env -s pwshis therefore ASCII only.$env:cannot spell go through[System.Environment]::SetEnvironmentVariable.{{takes the encoded form, becausetext/templatewould read it as an action.AGENTS.md: one line on the ASCII rule..github/workflows/ci.yml: a step inwindows-regressionsruns the new Windows test.Why
PowerShell decodes the output of a native command with the console's output code page, and the hook can switch the console to UTF-8 only after that output has been decoded. On a Chinese Windows (code page 936) the raw UTF-8 that
vfox activateprinted for PATH, the other variables and the vfox path came out garbled, so the session started with broken entries and errors. The$OutputEncodingline in the hook (#117) runs too late to help the first activation.The old escaper was also wrong on any system:
it'sbecame'it's'`, which PowerShell cannot parse, and tabs and newlines became backtick sequences that a single-quoted literal does not expand.For values without an apostrophe or a control character, the output is the same as before. I checked this against the old function for a set of ASCII values. Shells that are already open keep the old hook until they are restarted.
Tests
internal/shell/powershell_test.go,cmd/commands/activate_test.go) check that the output is ASCII and that the escaped text evaluates to the original value. The helper that evaluates the two forms follows PowerShell's rules; the Windows test below runs real PowerShell.TestPowerShellSessionKeepsNonASCIIPath(Windows only,cmd/commands/powershell_cli_test.go) runs Windows PowerShell and pwsh 7 with the console on code page 936, a non-ASCII PATH entry, and a vfox executable under a non-ASCII directory. It checks that the PATH entry survives, that the hook finishes, and that no error is reported.鐢ㄦ埛宸ュ叿and the vfox path as涓枃/bin/vfox.exe, which PowerShell reported as not recognized, in both shells.buildVfoxinnushell_cli_test.gonames the binaryvfox.exeon Windows. It had only ever run on Unix.windows-regressionsstep is there because the build matrix uses fail-fast: once one leg fails, the other legs are cancelled, and a cancelled Windows leg cannot be rerun on its own while the failure stands.The first two commits are red on purpose: the reproduction and the CI step. Upstream squash-merges, so they will not reach
main.Not covered: the code page is simulated on the runner.
[Console]::OutputEncodingis set to 936, and the test checks that it took effect. A real Chinese Windows installation has not been tested. Other shells are not changed.